-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix bug not to respect symbolic linked rules, if target is a directory or another symbolic link #2513
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 718c356 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| return | ||
| } | ||
|
|
||
| const fullPath = path.resolve(entry.parentPath || dirPath, entry.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code relies on a non-standard parentPath property on Dirent objects. parentPath isn’t part of the standard Dirent interface, which could lead to inconsistencies outside of your tests. Consider passing the directory path explicitly instead.
| const fullPath = path.resolve(entry.parentPath || dirPath, entry.name) | |
| const fullPath = path.resolve(dirPath, entry.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parentPath should be used. ref: #2405 (comment)
| const resolvedTarget = path.resolve(path.dirname(fullPath), linkTarget) | ||
|
|
||
| // Check if the target is a file | ||
| const stats = await fs.stat(resolvedTarget) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using fs.stat to check if the target is a symlink may be ineffective since fs.stat follows symlinks. To accurately detect a nested symlink, consider using fs.lstat instead.
| const stats = await fs.stat(resolvedTarget) | |
| const stats = await fs.lstat(resolvedTarget) |
mrubens
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thank you for cleaning this up 🙏
|
Thank you for prompt merge! Looking forward to the upcoming release 🤞 |
…y or another symbolic link (RooCodeInc#2513) * read symbolic linked dir and files recursively * add symlinked dir and nested symlink test case for custom-instructions * enhance comments * add changeset
Context
This PR fixes a bug not to respect the following symbolic links under
.roo/rulesdirectory.Implementation
Enhance
readTextFilesFromDirectoryfunction with two helper functions which are called recursively:So that recursive symbolic link or directory structure can be respected.
MAX_DEPTH = 5 is also set to avoid cyclic symbolic links and infinite loops.
Screenshots
Given this rules directory structure
Preview System Promptin RooCode project will look like:How to Test
As described in ScreenShots section.
Get in Touch
I'm in the RooCode Discord Server as
@taisukeoe. Please feel free to contact me if any.Important
Fixes bug in
readTextFilesFromDirectoryto handle symbolic links to directories and other symbolic links in.roo/rules, with tests added.readTextFilesFromDirectoryto handle symbolic links to directories and other symbolic links in.roo/rules.MAX_DEPTH = 5to prevent cyclic symbolic links and infinite loops.resolveSymLinkandresolveDirectoryEntryto recursively resolve symbolic links and directories incustom-instructions.ts.custom-instructions.test.tsto test new symbolic link handling behavior.This description was created by
for 718c356. It will automatically update as commits are pushed.